-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Fix ScriptTagHelper regression by checking for existing content before processing importmaps #63155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Greetings human! You've submitted a PR that modifies code that is shared with https://github.com/dotnet/runtime . Please make sure you synchronize this code with the changes in that repo! |
…sp-importmap Co-authored-by: MackinnonBuck <[email protected]>
// No importmap found. Only suppress output if this was intended to be | ||
// an automatically generated importmap (i.e., when asp-importmap was used). | ||
// If the user provided explicit content without asp-importmap, let it render as-is. | ||
if (ImportMap != null || context.AllAttributes.ContainsName(ImportMapAttributeName)) | ||
{ | ||
output.SuppressOutput(); | ||
return; | ||
} | ||
// Let the tag render as-is by continuing with normal processing | ||
// Don't return here, let normal attribute copying happen | ||
} | ||
else | ||
{ | ||
output.TagName = "script"; | ||
output.TagMode = TagMode.StartTagAndEndTag; | ||
output.Attributes.SetAttribute("type", "importmap"); | ||
output.Content.SetHtmlContent(importMap.ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the right fix.
If the goal is to do nothing if there is already content on the script, then what should happen is:
- Call
var originalContent = await output.GetChildContentAsync()
. - Check for content.IsEmptyOrWhitespace
- Set the import map only in that situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we still want to process all <script type="importmap"></script>
but the fix is only to avoid replacing existing content if there's already data the developer put it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 15776b3. I've implemented the approach you suggested:
- Call
await output.GetChildContentAsync()
to get existing content - Check if content is empty or whitespace using
IsEmptyOrWhiteSpace
- If content exists, preserve it and return early
- If no content exists, apply the normal importmap logic
The fix now properly processes all <script type="importmap"></script>
tags but avoids replacing existing content when developers have provided their own importmap data. I've maintained backward compatibility by keeping the original Process
method that calls the new ProcessAsync
method.
…tent Co-authored-by: javiercn <[email protected]>
public override void Process(TagHelperContext context, TagHelperOutput output) | ||
{ | ||
ProcessAsync(context, output).GetAwaiter().GetResult(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this override and add
*REMOVED*~override Microsoft.AspNetCore.Mvc.TagHelpers.ScriptTagHelper.Process(Microsoft.AspNetCore.Razor.TagHelpers.TagHelperContext context, Microsoft.AspNetCore.Razor.TagHelpers.TagHelperOutput output) -> void
to src/Mvc/Mvc.TagHelpers/src/PublicAPI.Unshipped.txt
instead
The ScriptTagHelper was incorrectly suppressing
<script type="importmap">
tags that contained explicit user-provided content, causing them to render as nothing in ASP.NET Core 9.0.Problem
When users wrote importmap scripts with explicit content like:
The ScriptTagHelper would completely suppress the output instead of preserving the user's content. This regression broke existing applications that relied on explicit importmap definitions.
Root Cause
The tag helper was being overly aggressive - it processed all script tags with
type="importmap"
and when noImportMapDefinition
was found in endpoint metadata, it would calloutput.SuppressOutput()
regardless of whether the user had provided explicit content.Solution
Modified the ScriptTagHelper to check for existing content first using
await output.GetChildContentAsync()
:This approach ensures that user-provided content is never replaced, while maintaining all existing functionality for automatic importmap generation.
Implementation Details
ProcessAsync
method that properly handles the async content checkingProcess
methodTesting
Added comprehensive tests to verify:
Fixes #58973.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.